switch to new package index, decompress into .tar in javascript #2024

garrettgu10 · 2024-04-15T19:17:59Z

how to test: bazel build //... && time ./bazel-bin/src/workerd/server/workerd test samples/pyodide-langchain/config.capnp | ts
Without this change: 27s loading packages, 155s CPU time, 63s total wall time
With this change: 1s loading packages, 100s CPU time, 40s total wall time

src/pyodide/internal/builtin_wrappers.js

dom96

See my comments, also can you describe how you've tested this in the PR description for future reference?

Some changes needed but accepting to unblock.

dom96 · 2024-04-19T10:25:05Z

src/pyodide/internal/loadPackage.js

+class ArrayBufferReader {
+  constructor(arrayBuffer) {
+    this.arrayBuffer = arrayBuffer;
+  }
+
+  read(offset, buf){
+    // buf is a Uint8Array
+    const size = this.arrayBuffer.byteLength;
+    if (offset >= size || offset < 0) {
+      return 0;
+    }
+    let toCopy = buf.length;
+    if (size - offset < toCopy) {
+      toCopy = size - offset;
+    }
+    buf.set(new Uint8Array(this.arrayBuffer, offset, toCopy));
+    return toCopy;
+  }
+}


Should tar.js instead be changed to expect an ArrayBuffer?

hmm, then we would have to expose an ArrayBuffer instead of a PackagesTarReader, not sure if we want that big of a refactor. This interface is meant to match exactly what PackagesTarReader does in JSG.

dom96 · 2024-04-19T10:26:24Z

src/pyodide/internal/loadPackage.js

+    loading.push(req);
+  }
+
+  console.log("Loading " + loading.join(", "));


Should these console.logs be removed?

I kept them there to match the original prints from the pyodide package loader. I'd prefer to keep them for now so we have more visibility on performance in testing.

dom96 · 2024-04-19T10:27:40Z

src/pyodide/internal/loadPackage.js

+
+  console.log("Loading " + loading.join(", "));
+
+  await Promise.all(loadPromises).then((buffers) => {


Nit

Suggested change

await Promise.all(loadPromises).then((buffers) => {

const buffers = await Promise.all(loadPromises);

dom96 · 2024-04-19T10:32:19Z

src/pyodide/internal/python.js

@@ -194,6 +195,7 @@ async function prepareWasmLinearMemory(Module) {
 }

 export async function loadPyodide(lockfile, indexURL) {
+  console.log("loading pyodide");


Suggested change

console.log("loading pyodide");

dom96 · 2024-04-19T10:32:25Z

src/pyodide/internal/loadPackage.js

+    }
+  });
+
+  console.log("Loaded " + loading.join(", "));


Suggested change

console.log("Loaded " + loading.join(", "));

dom96 · 2024-04-19T10:32:32Z

src/pyodide/internal/loadPackage.js

+    loading.push(req);
+  }
+
+  console.log("Loading " + loading.join(", "));


Suggested change

console.log("Loading " + loading.join(", "));

dom96 · 2024-04-19T10:33:50Z

src/pyodide/internal/setupPackages.js

+
+  /**
+   * A small bundle contains just a single package. The entire bundle will be overlaid onto site-packages.
+   * A small bundle can basically be thought of as a wheel.


Should it just be called addSingleWheel then?

dom96 · 2024-04-19T10:35:54Z

src/pyodide/internal/setupPackages.js

+   * @param {List<String>} soFiles A list of .so files contained in the big bundle
+   * @param {List<String>} requirements canonicalized list of packages to pick from the big bundle
+   */
+  addBigBundle(tarInfo, soFiles, requirements) {


I wonder why we have this split. Isn't addSmallBundle just the same as addBigBundle with a single requirement? Why separate them?

The structure of a big bundle is different from a small bundle. A big bundle contains one folder for each package, with each folder containing the structure of a wheel. A small bundle contains just the wheel for a single package, not contained within a folder.

dom96 · 2024-04-19T10:36:28Z

src/pyodide/internal/setupPackages.js

+
+  let LOAD_WHEELS_FROM_R2 = true;
+  let requirementsInBigBundle = new Set([...STDLIB_PACKAGES]);
+  if(bigTarInfo.children.size > 10) {


Suggested change

if(bigTarInfo.children.size > 10) {

if (bigTarInfo.children.size > 10) {

dom96 · 2024-04-19T10:38:18Z

src/pyodide/internal/setupPackages.js

 }

 function disabledLoadPackage() {
-  throw new Error("We only use loadPackage in workerd");
+  throw new Error("pyodide.loadPackage is disabled");


Suggested change

throw new Error("pyodide.loadPackage is disabled");

throw new Error("pyodide.loadPackage is disabled because packages are embedded in the binary");

* switch to new package index, decompress into .tar in javascript * switch to new index, perform full package installation in JavaScript * Address nits * Fix circular import issue

jasnell reviewed Apr 15, 2024

View reviewed changes

src/pyodide/internal/builtin_wrappers.js Outdated Show resolved Hide resolved

jasnell reviewed Apr 15, 2024

View reviewed changes

src/pyodide/internal/builtin_wrappers.js Outdated Show resolved Hide resolved

garrettgu10 force-pushed the ggu/tar-gz-wheels branch 2 times, most recently from 0ffbb12 to 653acab Compare April 16, 2024 17:09

garrettgu10 marked this pull request as ready for review April 16, 2024 21:49

garrettgu10 requested review from a team as code owners April 16, 2024 21:49

garrettgu10 requested review from a-robinson and hoodmane April 16, 2024 21:49

garrettgu10 force-pushed the ggu/tar-gz-wheels branch from 653acab to 1400739 Compare April 16, 2024 21:50

garrettgu10 requested a review from dom96 April 17, 2024 15:46

garrettgu10 force-pushed the ggu/tar-gz-wheels branch from 1400739 to 5768cef Compare April 17, 2024 18:25

garrettgu10 requested a review from a team as a code owner April 17, 2024 18:25

garrettgu10 force-pushed the ggu/tar-gz-wheels branch from 5768cef to 60ed77d Compare April 18, 2024 17:58

dom96 approved these changes Apr 19, 2024

View reviewed changes

garrettgu10 force-pushed the ggu/tar-gz-wheels branch from 60ed77d to 8810715 Compare April 22, 2024 18:07

garrettgu10 added 3 commits May 6, 2024 13:59

switch to new package index, decompress into .tar in javascript

cc5ae97

switch to new index, perform full package installation in JavaScript

9dd0e1d

Address nits

74ba19f

garrettgu10 force-pushed the ggu/tar-gz-wheels branch from 0f63bb7 to 74ba19f Compare May 6, 2024 18:59

Fix circular import issue

5559c51

garrettgu10 force-pushed the ggu/tar-gz-wheels branch from a7b8f61 to 5559c51 Compare May 7, 2024 15:41

garrettgu10 merged commit 9bb3e21 into main May 7, 2024
10 checks passed

garrettgu10 mentioned this pull request May 8, 2024

Convert wheels to tar.gz format cloudflare/pyodide-build-scripts#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to new package index, decompress into .tar in javascript #2024

switch to new package index, decompress into .tar in javascript #2024

garrettgu10 commented Apr 15, 2024 •

edited

Loading

dom96 left a comment

dom96 Apr 19, 2024

garrettgu10 Apr 19, 2024

dom96 Apr 19, 2024

garrettgu10 Apr 19, 2024

dom96 Apr 19, 2024

dom96 Apr 19, 2024

dom96 Apr 19, 2024

dom96 Apr 19, 2024

dom96 Apr 19, 2024

dom96 Apr 19, 2024

garrettgu10 Apr 19, 2024

dom96 Apr 19, 2024

dom96 Apr 19, 2024


		console.log("Loading " + loading.join(", "));

		await Promise.all(loadPromises).then((buffers) => {

	await Promise.all(loadPromises).then((buffers) => {
	const buffers = await Promise.all(loadPromises);

	if(bigTarInfo.children.size > 10) {
	if (bigTarInfo.children.size > 10) {

	throw new Error("pyodide.loadPackage is disabled");
	throw new Error("pyodide.loadPackage is disabled because packages are embedded in the binary");

switch to new package index, decompress into .tar in javascript #2024

switch to new package index, decompress into .tar in javascript #2024

Conversation

garrettgu10 commented Apr 15, 2024 • edited Loading

dom96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettgu10 commented Apr 15, 2024 •

edited

Loading